Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dev gui - Darkmode using HSL Hue #630

Merged
merged 228 commits into from
Nov 25, 2024
Merged

Conversation

gigamaster
Copy link
Contributor

What type of PR is this? (check all applicable)

  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • ♻️ Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Description

Feature: Improve overall app design (look and feel) #539

  1. Images svg optimization.

  2. Darkmode using HSL Hue degree on the color wheel from 0 to 360 ie. 0 is red, 120 is green, and 214 is blue.
    Default css var --hue: 214;

Todo : add an input range under "dark theme" in main menu and store Hue value in localStorage.

Related Tickets & Documents

WIP #539

Mobile & Desktop Screenshots/Recordings

livecodes-ui-wip

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentations?

  • πŸ““ docs (./docs)
  • πŸ“• storybook (./storybook)
  • πŸ“œ README.md
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

No.

[optional] What gif best describes this PR or how it makes you feel?

Dark Mode minimalist theming. Todo : rename CSS vars and improve colour scheme with high visual contrast.

styles/app.scss - change theme color HSL Hue degree on the color wheel from 0 to 360 ie. 0 is red, 120 is green, and 214 is blue.
Copy link

netlify bot commented Aug 27, 2024

βœ… Deploy Preview for livecodes ready!

Name Link
πŸ”¨ Latest commit feefddd
πŸ” Latest deploy log https://app.netlify.com/sites/livecodes/deploys/6744fc4590587a000844c84c
😎 Deploy Preview https://deploy-preview-630--livecodes.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hatemhosny
Copy link
Collaborator

Thank you @gigamaster
That's very nice πŸ‘Œ

I will have a good look and let you know.

Meanwhile, please run npm run fix to fix linting and formatting errors. Then we can look later into failing e2e tests.

Very grateful. Thanks.

@gigamaster
Copy link
Contributor Author

Thank you @gigamaster That's very nice πŸ‘Œ

I will have a good look and let you know.

Meanwhile, please run npm run fix to fix linting and formatting errors. Then we can look later into failing e2e tests.

Very grateful. Thanks.

Hi @hatemhosny

I was surprise that the Draft pull request to share the work-in-progress did trigger automated workflows 🀨
The automated development workflows are great and the "Deploy Preview" is really 😎
The purpose of this Draft PR was to seek feedback, discuss design decisions, iterate, and refine the changes before a pull request.
For example, moving UI icons to CSS using mask to make it easier to customize colors and sizes.
Use HSL Hue, an input range and save preferences to localStorage or make custom color palettes.
Then, delete code comments based on decisions made and run tests.
Have fun. Cheers ^_^/

@hatemhosny
Copy link
Collaborator

No worries, @gigamaster

I understand this is still WIP. I did trigger the automated checks just to get an idea.

I will go through the changes and design decisions and give feedback.

One note for now:
A better way than manually storing this setting to localStorage, is to add a config option (e.g. themeAccent or themeHue). This automatically saves it locally, and also allows other scenarios, like setting it in query params and setting/getting it in SDK even during runtime.
That's an easy change that I can guide implementing.

Overall, I really like what you are doing here. I'm a bit busy with some personal commitments for the next few days. I will have a good look when I have a chance and get back to discuss with you here.

Thank you.

@hatemhosny
Copy link
Collaborator

Hi @gigamaster

I had a good look around. I very much like your approach.
Using HSL hue and CSS icons are very neat.
I like the ability to change the colors by changing a single CSS variable.
Great work. Thanks a lot.

I do have some comments. A few are related to UI & colors (e.g. contrast of text/images with background, etc.). I think I would keep these till you finish because you are probably going to improve them anyways. You are much better than me in this area.

Other comments:

  • I planned doing this for a while, maybe it's time now. I want to change all clickable elements to <button> elements (to improve semantics, improve accessibility and keyboard navigation). We can still use CSS icons by either changing the spans to buttons or putting the spans inside the buttons (which is still valid HTML).
  • e2e tests failed mainly because clicking the run button using this selector ([alt="Run"]) no longer works. We need to change the selector to something that would match any of these 2 buttons: this and that. Maybe add a data attribute.
  • Would you elaborate why we need to add these gitattributes? What was the problem you faced? Would * text=auto be enough? Please note that I use Windows, but also want to allow contributors using other OSes to work with no problems.
  • One last stylistic comment (just a personal opinion): I preferred darker color for menus and models like you did here, while keeping the hue for accents like hover, active items, buttons, etc. What do you think?

Otherwise, please go on. I do like your approach.
Obviously, please feel free to ask if you have any inquiries.
Thank you.

@gigamaster
Copy link
Contributor Author

Hi @hatemhosny

I'm glad for the feedback.

I had a good look around. I very much like your approach. Using HSL hue and CSS icons are very neat. I like the ability to change the colors by changing a single CSS variable. Great work. Thanks a lot.

I do have some comments. A few are related to UI & colors (e.g. contrast of text/images with background, etc.). I think I would keep these till you finish because you are probably going to improve them anyways. You are much better than me in this area.

Ok then lets try to keep it simple...

Other comments:

  • I planned doing this for a while, maybe it's time now. I want to change all clickable elements to <button> elements (to improve semantics, improve accessibility and keyboard navigation). We can still use CSS icons by either changing the spans to buttons or putting the spans inside the buttons (which is still valid HTML).

Great, I'll dig into the code to see how changes can be made.

  • e2e tests failed mainly because clicking the run button using this selector ([alt="Run"]) no longer works. We need to change the selector to something that would match any of these 2 buttons: this and that. Maybe add a data attribute.

Noted.

  • Would you elaborate why we need to add these gitattributes? What was the problem you faced? Would * text=auto be enough? Please note that I use Windows, but also want to allow contributors using other OSes to work with no problems.

Oohh... gitattributes was merged from my workspace mockup/prototype/test : android -> old imac -> windows
In general, if it works on the old machine, it should work fine everywhere. Guess, it can be removed.
I did not find any particular issue so far, livecodes setup is highly efficient.

  • One last stylistic comment (just a personal opinion): I preferred darker color for menus and models like you did here, while keeping the hue for accents like hover, active items, buttons, etc. What do you think?

Otherwise, please go on. I do like your approach. Obviously, please feel free to ask if you have any inquiries. Thank you.

Dark/light mode implements a color-layer for the livecodes app and color-hue on elements only.
I tried CSS variables inheritance and fallback from editor, but it turned out to be confusing since users can choose a different editor... Also, the settings menu is split with a new settings-ui menu.
The menus could be over the top i.e : file, settings, utils, help, more on this later.
Right now, the menu setttings and settings-ui could be placed next to the logo
while keeping Run and Share on the left.

Livecodes UI Modal

livecodes-ui-modal

Livecodes UI Dialog

livecodes-ui-dialog

Livecodes UI Menu Settings

livecodes-ui-menu

@hatemhosny
Copy link
Collaborator

Oh wow!
That looks beautiful πŸ’―

I tried CSS variables inheritance and fallback from editor, but it turned out to be confusing since users can choose a different editor

Yes, currently users can choose a light editorTheme with a dark app theme. We might change this later, but this is the current behavior.

Also, the settings menu is split with a new settings-ui menu.

This is reasonable. Actually the left column of the "App menu" is related to the current project (except the login/logout link). While the right column is related to overall app settings. So it may be reasonable to split them to 2 menus: "Project" and "App Settings" (also with login link)

The menus could be over the top i.e : file, settings, utils, help, more on this later.
Right now, the menu setttings and settings-ui could be placed next to the logo
while keeping Run and Share on the left.

I will need to see a demo of this to make up my mind. Please note that LiveCodes does not provide multi-file support, so having a menu called "file" would probably not be suitable. "Project" would be a more suitable name.

Whenever possible, please push your changes, so that I can try it in the deploy preview. It does not need to pass the tests for now.

Thank you :)

@hatemhosny
Copy link
Collaborator

@gigamaster
One more thing that is not urgent at all, but I thought it might be useful to tell you in case you want to take it into consideration.
There is a quite significant work being done here to provide i18n support in LiveCodes so that the app would be multi-lingual. That feature is approaching to be ready.

A lot of LiveCodes users are Arabic speakers. So it would be great if we can also offer RTL layout.

This does not need to be in this PR. Just wanted to give you heads up.

Thank you :)

@gigamaster
Copy link
Contributor Author

gigamaster commented Sep 2, 2024

@gigamaster One more thing that is not urgent at all, but I thought it might be useful to tell you in case you want to take it into consideration. There is a quite significant work being done here to provide i18n support in LiveCodes so that the app would be multi-lingual. That feature is approaching to be ready.

A lot of LiveCodes users are Arabic speakers. So it would be great if we can also offer RTL layout.

This does not need to be in this PR. Just wanted to give you heads up.

Thank you :)

Hi @hatemhosny

So, to avoid any issues with editor theme selection, dark mode applies only to the application UI layer. By the way, I also noticed that Luna console and notifications are configured with light theme by default - maybe this can be changed to match the selected editor dark theme and, or app darkmode !?

After a quick check of i18n support branch, it doesn't seem like it's a problem to adapt and merge the UI. You can focus on i18n and later merge dark mode. Let me know when it's ready for translations.

About RTL layout - css dir pseudo-class might do the trick, maybe similar to html.dark, ie. html[dir="rtl"] to provide optional direction. I'll check browser support and test.
https://caniuse.com/css-dir-pseudo
https://caniuse.com/?search=direction

@hatemhosny
Copy link
Collaborator

hatemhosny commented Sep 2, 2024

So, to avoid any issues with editor theme selection, dark mode applies only to the application UI layer.

Yes, let's keep it like this for now.

By the way, I also noticed that Luna console and notifications are configured with light theme by default

You are correct. I have opened a PR in your repo to fix that. You may want to customize notification colors here. You can get the active theme using getConfig().theme.

After a quick check of i18n support branch, it doesn't seem like it's a problem to adapt and merge the UI. You can focus on i18n and later merge dark mode. Let me know when it's ready for translations.

That's great. Thank you.

About RTL layout - css dir pseudo-class might do the trick, maybe similar to html.dark, ie. html[dir="rtl"] to provide optional direction. I'll check browser support and test.
https://caniuse.com/css-dir-pseudo
https://caniuse.com/?search=direction

@shadeed has a great guide about RTL styling here: https://rtlstyling.com/posts/rtl-styling

Thank you.

@gigamaster
Copy link
Contributor Author

I'll merge the fix ToolsPane and update.
Thanks for the prompt reply and @shadeed guide, it's really helpful ^_^/

fix(ToolsPane): fix console themes
add menus project, settings, help
remove CodeRunButton
mobile orientation landscape
assets base64 scroll
@gigamaster
Copy link
Contributor Author

gigamaster commented Sep 6, 2024

I'll merge the fix ToolsPane and update. Thanks for the prompt reply and @shadeed guide, it's really helpful ^_^/

UI darkmode
add menus project, settings, help
remove CodeRunButton
mobile orientation landscape
assets base64 scroll

@hatemhosny
Copy link
Collaborator

hatemhosny commented Sep 7, 2024

Thank you @gigamaster
I had a look around

UI darkmode
add menus project, settings, help

Yes, I see now what you mean. I think this is a nice layout. That's more organized.
We can continue this pathway.

The hint tooltip covers the first menu item. It needs to be moved.
May be change class="menu hint--bottom" to class="menu hint--right"

remove CodeRunButton
mobile orientation landscape

This was needed for small screens because of lack of space in the top bar. I see you have not yet implemented the design for mobile portrait orientation. So, I'll just wait to see how this evolves. We also have now the login button (which is no longer in menu).

assets base64 scroll

I'm not sure this is useful. It might even be a bit confusing for users to scroll through the very long data urls.
I cannot find a delete button for assets. Unrecognized file icon (e.g. csv) does not have enough contrast to background.

The embedded playgrounds now break (demo).
In embedded playgrounds I move the logo to the right and use it to open the current project in the full standalone app.
The selector is no longer able to find the element to it crashes. You can find selectors here. You probably need to preserve these or replace them with valid selectors.

The dark loading screen should work only in dark mode. At that time the app data have not been loaded yet.
We faced this issue while implementing translation for the loading text. The code for i18n feature provides a solution for that which we can use for this once the other is merged.

Please go on. I like the direction you are going.
Thank you.

@gigamaster
Copy link
Contributor Author

Thank you @gigamaster I had a look around

UI darkmode
add menus project, settings, help

Yes, I see now what you mean. I think this is a nice layout. That's more organized. We can continue this pathway.

The hint tooltip covers the first menu item. It needs to be moved. May be change class="menu hint--bottom" to class="menu hint--right"

remove CodeRunButton
mobile orientation landscape

This was needed for small screens because of lack of space in the top bar. I see you have not yet implemented the design for mobile portrait orientation. So, I'll just wait to see how this evolves. We also have now the login button (which is no longer in menu).

assets base64 scroll

I'm not sure this is useful. It might even be a bit confusing for users to scroll through the very long data urls. I cannot find a delete button for assets. Unrecognized file icon (e.g. csv) does not have enough contrast to background.

The embedded playgrounds now break (demo). In embedded playgrounds I move the logo to the right and use it to open the current project in the full standalone app. The selector is no longer able to find the element to it crashes. You can find selectors here. You probably need to preserve these or replace them with valid selectors.

The dark loading screen should work only in dark mode. At that time the app data have not been loaded yet. We faced this issue while implementing translation for the loading text. The code for i18n feature provides a solution for that which we can use for this once the other is merged.

Please go on. I like the direction you are going. Thank you.

Hi @hatemhosny

The main idea is to make the user interface layout match the modern web apps design that Internet users are familiar with.
If visuals are important, we remember more about how a web app helped to achieve our goals, and how easy it was to do it. Let's say you are on the go, on a train or plane, and want to elaborate an idea, the choice would be pencil and paper or "livecodes". The goal is then to maximize the area of code and minimize the menus to provide quick access to features (spacing items to later customize with icons and keyboard shortcuts). I thought a tools menu could be useful to move the asset manager, code snippets and allow other add-ons, e.g. CSS color and unit conversion, cloud storage (link, modal or popup), more on this later.

The base64 asset scrolling was an attempt to fix the very long string after adding an image. While I try to avoid touching the core files as much as possible, I noticed that modals have a different HTML element structure that can't be solved by CSS alone, same for tabindex and flex ordering to facilitate LTR and RTL.

Some buttons don't show up, but I find them useful, for example, split and full screen.
Are they supposed to be used only on the embedded playground?

Thanks for reporting the issues - I'll check that out further.
Have fun ^_^/

@gigamaster
Copy link
Contributor Author

gigamaster commented Sep 7, 2024

  • Todo nav - menu

ConsistencyNot conventional
Use hr instead of the separator role to ensure accessibility across all devices.

Validation Error: Element hr not allowed as child of element ul
HR shouldn't be recommended since it's invalid.

However, the li[role="separator"] is allowed in HTML 5.2
Ref.:
https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-editor/
https://www.w3.org/TR/html52/grouping-content.html#the-li-element

Issue:
whatwg/html#9247

listitem role (default - do not set), option, presentation, radio, separator, tab or treeitem.

@hatemhosny
Copy link
Collaborator

hatemhosny commented Sep 7, 2024

The main idea is to make the user interface layout match the modern web apps design that Internet users are familiar with.
If visuals are important, we remember more about how a web app helped to achieve our goals, and how easy it was to do it. Let's say you are on the go, on a train or plane, and want to elaborate an idea, the choice would be pencil and paper or "livecodes". The goal is then to maximize the area of code and minimize the menus to provide quick access to features (spacing items to later customize with icons and keyboard shortcuts). I thought a tools menu could be useful to move the asset manager, code snippets and allow other add-ons, e.g. CSS color and unit conversion, cloud storage (link, modal or popup), more on this later.

I do agree on the concept and approach. I think this is indeed more organized.

The base64 asset scrolling was an attempt to fix the very long string after adding an image. While I try to avoid touching the core files as much as possible, I noticed that modals have a different HTML element structure that can't be solved by CSS alone, same for tabindex and flex ordering to facilitate LTR and RTL.

I do not mind introducing some changes in HTML, so long that they do not break the app.
I just mean that showing the full data url is not useful to the user and may be confusing. No body would actually need to read the full data URL in contrast to other regular URLs which might be useful (e.g. when uploaded to github pages)
Another way of doing this would be to trim the URL so that it spans only 1 line, like the current implementation.
I also tried to make it look similar to the project list screen (Open ...), for familiarity and consistency.

image

The embedded playgrounds now break (demo).

It now works. Thank you.

Some buttons don't show up, but I find them useful, for example, split and full screen.
Are they supposed to be used only on the embedded playground?

Yes. Embedded playgrounds have limited space (inside the embedding page). So it would be useful to see the playground in full screen to have more room without taking up more space from the embedding page. In contrast, the full app already takes the full view port and can be easily viewed in full screen by pressing F11 (which is not available for embeds).
Same goes for the split toggle button. It is easier to manually resize in the full app.

On the other hand, all menus and login button should not be shown in embeds. I do not even attach event handlers to them.
It should be focused on just showing the UI necessary to edit some code and see the result. In case the user wants to do more, he can click on the logo (with the hint: edit in LiveCodes), which would take him to the full app with his project loaded.

This is the current view for embeds. We should hide all menus and the login button:

image

I noticed that changing the editor causes some flacky movement in the editor toolbar below:

Screenity.video.-.Sep.7.2024.mp4

However, the li[role="separator"] is allowed in HTML 5.2

I do like the menu separators. They make things a lot more organized and also look great.
Thank you.

@hatemhosny
Copy link
Collaborator

One more thing. The tooltip in the lowest menu item tries to appear below but overflows to the next column.
Maybe we should change all tooltips to appear on the right class="hint--right", like you did with the menu labels.
Also for changing language.

image

@gigamaster
Copy link
Contributor Author

gigamaster commented Sep 8, 2024

  • App Menu ( spacing + flex)
  • App Menu tooltip (language-item hint left position)
  • Embed playground (remove menus + login)
  • Console tests (result, fail css var)
  • Console luna count (text color)
  • Fixed editor toolbar (position absolute)
  • Reverse asset manager + add action delete
  • Removed .gitattributes (CRLF/LF)

WIP

  • Dark & Light mode color scheme
  • CSS vars
  • LTR - RTL
    • Extra margin in flexbox - Browser bug ??
    • Menu select-editor
    • Position absolute (left-right)
  • i18n
  • Tests

@hatemhosny
Copy link
Collaborator

@zyf722 @gigamaster

Maybe showing ellipsis is better

I did use your approach. Thank you.
Now language names in menus and editor titles overflow with ellipsis. A title attribute shows the full name.

The offset is better now, but the menu items are still not aligned properly. Any ideas?

The menus used position: absolute, which made it difficult to account for all variations: wide/narrow screens, different UI languages, ltr/rtl layouts. Some mal-alignment always occured, we just tried to adjust it as much as possible.
Now I changed the approach to use CSS anchor positioning. It now works with all variants with a such simpler code and no media queries. However, it is a new CSS feature with poor browser support. So, I added a polyfill in case it is not natively supported.

CSS min-max height has been removed in the modal, which creates a large empty space in desktop screens.

You are right. I did that to improve some modals with long content that required scrolling.
I added a fix. It should be better now.

@hatemhosny
Copy link
Collaborator

  • added fixes for cross-browser support (mainly colorTheme in firefox and menus in safari)

By the way, we have a free open-source account on browserstack.com. It is great for cross-browser tesing.
If you are interested I can send you an invitation to join our team account there.

  • changed the unicode arrow (the one we used appeared in android in a blue box)

image

after fix:

image

@hatemhosny
Copy link
Collaborator

@gigamaster @zyf722

I'm ready to merge if you have no further comments.

@gigamaster
Copy link
Contributor Author

gigamaster commented Nov 24, 2024

Using the CSS anchor positioning feature is great, but it's a bit risky.
Who knows when the Firefox and Safari teams will make it available.
Run Firefox Dev locally, the polyfill works on the first load, as soon as you change language it's lost.
Might try this on browserstack.com.
That said, just merge it ! It might be easier to update translations in Lokalise.

Copy link

sonarcloud bot commented Nov 25, 2024

@hatemhosny
Copy link
Collaborator

Run Firefox Dev locally, the polyfill works on the first load, as soon as you change language it's lost.

I removed the polyfill.
It was not reliable enough, lacked many features and had a huge file size (~100kb gzipped).

Alternatively, I added our original code as fallback

@supports not (anchor-name: --app-menu-project) {
  // original code
}

@hatemhosny hatemhosny merged commit 6d265a0 into live-codes:develop Nov 25, 2024
11 checks passed
Copy link
Contributor

livecodes-ci bot commented Nov 25, 2024

i18n Actions

Source PR has been merged into the default branch.

Maintainers can comment .i18n-update-push to trigger the i18n update workflow and push the changes to Lokalise.

@hatemhosny
Copy link
Collaborator

Merged at last πŸŽ‰

@gigamaster
I'm very grateful for the outstanding work.
This ended up much better than my expectations when I first thought about updating the UI.
Thank you.

@zyf722
We can now start working on translations and sync with Lokalise.

@hatemhosny
Copy link
Collaborator

.i18n-update-push

Copy link
Contributor

livecodes-ci bot commented Nov 26, 2024

i18n Actions: .i18n-update-push

Localization updated and pushed to Lokalise.

Name Description
New Branch for i18n i18n/gigamaster/dev-gui
Last Commit SHA 6d265a0

Maintainers can comment .i18n-update-pull after translation is done to trigger the i18n pull workflow and pull the changes back to Github.

@gigamaster
Copy link
Contributor Author

Merged at last πŸŽ‰

Congratulations to all of you for your positive thinking,
which enables you to rise to any challenge and strive for excellence.
People like you inspire others to improve their performance.

You have truly understood the meaning of teamwork.
The way this team has worked is the best example,
for juniors and seniors alike, of effective collaboration
and the best results.

Thank you for all your hard work, support and inspiration.
Well done to each and every one of you ^_^/

@hatemhosny
Copy link
Collaborator

Merged at last πŸŽ‰

Congratulations to all of you for your positive thinking, which enables you to rise to any challenge and strive for excellence. People like you inspire others to improve their performance.

You have truly understood the meaning of teamwork. The way this team has worked is the best example, for juniors and seniors alike, of effective collaboration and the best results.

Thank you for all your hard work, support and inspiration. Well done to each and every one of you ^_^/

@gigamaster @zyf722

We truly are forming an excellent team.
What has been achieved over the past few months would not have been possible without such consistent high quality work and very constructive collaboration.

That's a team I love to continue working with.
Thank you ❀️

@zyf722
Copy link
Contributor

zyf722 commented Nov 26, 2024

@gigamaster @hatemhosny

Thank you both for all the effort you’ve put in! I am truly grateful for the opportunity to participate in this process that significantly enhances the whole application over the past few months.

It’s been a great and pleasant experience collaborating with both of you, and I've learned a lot from your expertise and precious teamwork.

Wish you all the best, and hope we continue to collaborate on the next focuses! πŸš€

@hatemhosny
Copy link
Collaborator

updated README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants